fix(table-core): guard process.env.NODE_ENV checks for bundler-less environments - #6567
fix(table-core): guard process.env.NODE_ENV checks for bundler-less environments#6567koreahghg wants to merge 1 commit into
Conversation
…nvironments Raw process.env.NODE_ENV reads in ~14 dev-only debug/validation checks survive unguarded into the published ESM build. Any environment with no process global (e.g. vanilla JS loaded via an import map, no bundler or Node.js runtime) throws ReferenceError: process is not defined the first time one of these checks runs (GH TanStack#6078). Add a shared isDevelopmentEnv() helper that checks typeof process first, and route every call site through it. Behavior/semantics are unchanged (still === 'development', not flipped to !== 'production'). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MILLERMARRU
left a comment
MILLERMARRU
left a comment
There was a problem hiding this comment.
Checked the full diff, all 14 raw process.env.NODE_ENV reads got routed through isDevelopmentEnv() (grepped for leftover unguarded reads across the changed files, none left). The typeof process !== 'undefined' guard is the right fix for the actual bug (#6078), a ReferenceError on process itself, not a value-checking issue.
Agreed with keeping this scoped to just the crash. The development vs !== production semantics debated on #6445 is a separate, more contentious question (default-on vs default-off checks in unconfigured production builds) and bundling it here would've blocked a straightforward crash fix behind an unrelated design decision. Good call leaving that for #6445 to resolve on its own.
One thing I'd double check before merge: isDevelopmentEnv() itself needs to live somewhere that doesn't get tree-shaken oddly given it's now imported into ~10 different feature files, worth confirming the dist/utils.js check you did in the description also holds for the tree-shaken per-feature entry points if table-core publishes those separately, not just the main bundle.
|
Good catch! checked this by actually building the branch ( Only
So the guard holds across every per-feature entry point that actually uses it — no unguarded reads leak into any of the published bundles. |
2 participants
🎯 Changes
Fixes #6078.
table-core's ~14 dev-only debug/validation checks readprocess.env.NODE_ENVdirectly. That raw read survives unguarded into the published ESM build (unbundle: true, nodefineintsdown.config.ts), so any environment with noprocessglobal — vanilla JS loaded via an import map, or any other bundler-less/non-Node setup — throwsReferenceError: process is not definedthe moment one of these checks runs (e.g. simply constructing a table withdebugTable: true).Added a single
isDevelopmentEnv()helper inutils.tsthat checkstypeof process !== 'undefined'before readingprocess.env.NODE_ENV, and routed all 14 call sites through it. Behavior is otherwise unchanged — still=== 'development', not flipped to!== 'production'(that was the debated part of the related draft PR #6445, which is why it's stuck; this PR intentionally only fixes the crash and leaves that semantic question alone).Verified the fix actually reaches the published artifact: after building, the only
process.envreference left indist/utils.jsis the guarded one insideisDevelopmentEnv().✅ Checklist
pnpm run test:pr(ran the equivalent steps directly:vitest runfor@tanstack/table-core— 1275/1275 passing, including new regression tests — plustsc --noEmit,eslint, and atsdownbuild; the fullnx affectedpipeline timed out locally on an unrelatedtest:sherif/test:knippass over the whole workspace).🚀 Release Impact